Skip to content

FFI: get scalars from arrays - #8896

Open
myrrc wants to merge 2 commits into
developfrom
myrrc/ffi-getter-session
Open

FFI: get scalars from arrays#8896
myrrc wants to merge 2 commits into
developfrom
myrrc/ffi-getter-session

Conversation

@myrrc

@myrrc myrrc commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • Add vx_array_get_scalar method for retrieving vx_scalar.
  • Add scalar's primitive, utf8, bool, and binary getters generated from a macro.
  • Replace decimal scalar creation functions with a macro.
  • Remove vx_array's primitive getters in favor of obtaining a vx_scalar.
  • Add C++ API support for getting array's Scalar at index.
    Error propagation in C bindings #7483

@myrrc myrrc added the changelog/break A breaking API change label Jul 22, 2026
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from 3945f76 to c305a0e Compare July 22, 2026 12:32
@myrrc
myrrc requested a review from 0ax1 July 22, 2026 12:32
@myrrc myrrc changed the title Pass vx_session to ffi scalar getters" Pass vx_session to ffi scalar getters Jul 22, 2026
@myrrc myrrc added the lang/c Relates to the Vortex C (FFI) API label Jul 22, 2026
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from c305a0e to b1ac575 Compare July 22, 2026 12:34
@github-actions

github-actions Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Polar Signals Profiling Results

Latest Run

Status Commit Job Attempt Link
🟢 Done b1ac575 1 Explore Profiling Data
Previous Runs (1)
Status Commit Job Attempt Link
🟢 Done 3945f76 1 Explore Profiling Data

Powered by Polar Signals Cloud

@github-actions

github-actions Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Verdict: No clear signal (low confidence)
Attributed Vortex impact: +2.5%
Engines: DataFusion Likely regression (+11.5%, medium confidence) · DuckDB No clear signal (-5.8%, low confidence)
Vortex (geomean): 0.954x ➖
Parquet (geomean): 0.942x ➖
Shifts: Parquet (control) -5.8% · Median polish -4.3%

How to read Verdict and Engines
  • Verdict: Overall PR-level signal after subtracting baseline drift estimated from Parquet control rows. It can be Likely improvement, Likely regression, or No clear signal.
  • Engines: Per-engine attribution. DataFusion is compared against DataFusion/Parquet controls; DuckDB is compared against DuckDB/Parquet controls. This answers whether each engine improved or regressed independently.
  • Confidence: Based on directional consistency, share of rows above the noise floor, and control-run noise.
datafusion / vortex-file-compressed (1.004x ➖, 0↑ 0↓)
name PR b1ac575 (ns) base 037b1a8 (ns) ratio (PR/base)
vortex_q00/datafusion:vortex-file-compressed 9774512 10016685 0.98
vortex_q01/datafusion:vortex-file-compressed 6448746 6241940 1.03
datafusion / parquet (0.901x ➖, 1↑ 0↓)
name PR b1ac575 (ns) base 037b1a8 (ns) ratio (PR/base)
vortex_q00/datafusion:parquet 20056827 21628324 0.93
vortex_q01/datafusion:parquet 🚀 4574698 5231454 0.87
duckdb / vortex-file-compressed (0.929x ➖, 0↑ 0↓)
name PR b1ac575 (ns) base 037b1a8 (ns) ratio (PR/base)
vortex_q00/duckdb:vortex-file-compressed 10144766 10577988 0.96
vortex_q01/duckdb:vortex-file-compressed 5933116 6588883 0.90
duckdb / parquet (0.986x ➖, 0↑ 0↓)
name PR b1ac575 (ns) base 037b1a8 (ns) ratio (PR/base)
vortex_q00/duckdb:parquet 23406979 23594326 0.99
vortex_q01/duckdb:parquet 9363326 9549441 0.98

No file size changes detected.

@myrrc
myrrc enabled auto-merge (squash) July 22, 2026 12:36
joseph-isaacs
joseph-isaacs reviewed Jul 22, 2026
View reviewed changes
Comment thread vortex-ffi/cinclude/vortex.h
joseph-isaacs
joseph-isaacs reviewed Jul 22, 2026
View reviewed changes
Comment thread vortex-ffi/src/array.rs Outdated
@0ax1
0ax1 removed their request for review July 23, 2026 13:47
@myrrc myrrc changed the title Pass vx_session to ffi scalar getters ffi: separate scalar execution and primitive getters Jul 28, 2026
@myrrc myrrc changed the title ffi: separate scalar execution and primitive getters FFI: separate scalar execution and primitive getters Jul 28, 2026
@myrrc myrrc changed the title FFI: separate scalar execution and primitive getters FFI: separate scalar execution from primitive getters Jul 28, 2026
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from b1ac575 to ea86234 Compare July 28, 2026 14:13
@myrrc
myrrc requested a review from joseph-isaacs July 28, 2026 14:13
joseph-isaacs
joseph-isaacs reviewed Jul 29, 2026
View reviewed changes
Comment thread vortex-ffi/src/scalar.rs
joseph-isaacs
joseph-isaacs reviewed Jul 29, 2026
View reviewed changes
Comment thread vortex-ffi/src/scalar.rs Outdated
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from ea86234 to 255042c Compare July 29, 2026 14:50
@myrrc
myrrc requested a review from joseph-isaacs July 29, 2026 14:51
@myrrc
myrrc marked this pull request as draft July 29, 2026 14:56
auto-merge was automatically disabled July 29, 2026 14:56

Pull request was converted to draft

@myrrc

myrrc commented Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've checked again, and I think we want a more gradual change.

We do want to be able to get a scalar for an array, but we can't replace current getters with an intermediate vx_scalar:

  1. for bool, we would pay the price of allocating a scalar for every item. This is non-performant, but acceptable, and can be solved by ffi: create bool arrays #8899
  2. For strings, however, we can't have an intermediate Scalar because the vx_view's now borrow from a scalar, and C++ api's bulk access would become UB.

I've removed the primitive accessors here.

@myrrc myrrc changed the title FFI: separate scalar execution from primitive getters FFI: get scalars from arrays Jul 30, 2026
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from 255042c to 49f87e7 Compare July 30, 2026 15:10
@myrrc myrrc added changelog/feature A new feature changelog/break A breaking API change and removed changelog/break A breaking API change changelog/feature A new feature labels Jul 30, 2026
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch 2 times, most recently from 36789e7 to 11f23f4 Compare July 30, 2026 15:43
@myrrc
myrrc marked this pull request as ready for review July 30, 2026 15:55
@myrrc
myrrc enabled auto-merge (squash) July 30, 2026 15:55
@myrrc
myrrc requested review from joseph-isaacs and removed request for joseph-isaacs July 30, 2026 15:56
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from 11f23f4 to 413207b Compare July 30, 2026 15:58
@myrrc
fix
b0ff733 
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
@myrrc
myrrc force-pushed the myrrc/ffi-getter-session branch from 413207b to b0ff733 Compare July 30, 2026 16:03
@myrrc myrrc added the lang/cpp Relates to the Vortex C++ API label Jul 30, 2026
@myrrc
fix
736068d 
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
joseph-isaacs
joseph-isaacs reviewed Jul 31, 2026
View reviewed changes
Comment thread vortex-ffi/cinclude/vortex.h
* Errors if "index" is out of bounds.
*/
const vx_scalar *
vx_array_get_scalar(const vx_session *session, const vx_array *array, size_t index, vx_error **error_out);

@joseph-isaacs joseph-isaacs Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does execution with a session, we should explain that

joseph-isaacs
joseph-isaacs reviewed Jul 31, 2026
View reviewed changes
Comment thread vortex-ffi/src/scalar.rs
Comment on lines 58 to 62
pub unsafe extern "C-unwind" fn vx_scalar_new_bool(
value: bool,
is_nullable: bool,
) -> *mut vx_scalar {
vx_scalar::new(Scalar::bool(value, Nullability::from(is_nullable)))

@joseph-isaacs joseph-isaacs Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unneeded, there can only be 3 values here

@joseph-isaacs

joseph-isaacs commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

for bool, we would pay the price of allocating a scalar for every item. This is non-performant, but acceptable, and can be solved by #8899
For strings, however, we can't have an intermediate Scalar because the vx_view's now borrow from a scalar, and C++ api's bulk access would become UB.

Can you point to the functions you mean here

onursatici
onursatici reviewed Jul 31, 2026
View reviewed changes
Comment thread lang/cpp/include/vortex/scalar.hpp
* std::string_view and BinaryView returned borrow from scalar and stay
* valid while scalar is valid.
*
* Throws if scalar type does not match T or value is Null.

@onursatici onursatici Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we say this method throws here but it does actually panic right? Shall we validate the dtype and validity and throw so we do what we say?

Comment thread vortex-ffi/cinclude/vortex.h

int64_t vx_array_get_i64(const vx_array *array, size_t index);

int64_t vx_array_get_storage_i64(const vx_array *array, size_t index);

@onursatici onursatici Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an equivalent to do what this method did now?

Comment thread lang/cpp/include/vortex/scalar.hpp
} else if constexpr (std::is_same_v<T, double>) {
return vx_scalar_get_f64(h);
} else {
static_assert(false, "f16 scalar get is not supported");

@onursatici onursatici Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we not support f16? I mean we do support constructing f16 arrays or scalars, but we can't get a scalar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joseph-isaacs joseph-isaacs joseph-isaacs left review comments
@onursatici onursatici onursatici left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

changelog/break A breaking API change lang/c Relates to the Vortex C (FFI) API lang/cpp Relates to the Vortex C++ API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@myrrc @joseph-isaacs @onursatici